Skip to content
This repository has been archived by the owner on May 22, 2019. It is now read-only.

Added a classname for the appInsights HOC's div #89

Closed
wants to merge 1 commit into from

Conversation

florianchevallier
Copy link

Just a way to fix #73

@florianchevallier
Copy link
Author

@pviotti could you please check this PR ? Thank you

@pviotti
Copy link
Contributor

pviotti commented May 14, 2019

Hi, sorry for the late reply. This project has been officially "adopted" by the AppInsights team, so the next version will be hosted in the ApplicationInsights-JS mono-repo, and it's currently available at this path: https://github.com/microsoft/ApplicationInsights-JS/tree/master/vNext/extensions/applicationinsights-react-js Soon we will be closing this project, deprecating the package and redirecting people to that repository.
So I suggest to port the changes of this PR to the code in that repository.

Regarding this PR, I'm not sure what you actually aim to solve by adding an arbitrary class name to the HoC div...

@florianchevallier
Copy link
Author

florianchevallier commented May 14, 2019

Hi, thanks for your answer and the clarification concerning the future of this repo.

Regarding this PR, giving an arbitrary class name to the HoC div helps solving this probleme ( #73 ).
In our app, the app has a 100% height / width on the divs, but adding another div as a HoC and not beeing able to style it easily breaks the layout of my app.
If you find a way to add a dynamic class name to the HoC, it would be great, but for now, adding this name will be really enough.

@pviotti
Copy link
Contributor

pviotti commented May 14, 2019

OK, understood. I think that adding an optional argument to the HoC function would be a cleaner solution, as proposed in this comment: #73 (comment)
I'll be closing this PR, but I encourage you to propose this change to the new codebase.
Thanks!

@florianchevallier
Copy link
Author

OK, understood. I think that adding an optional argument to the HoC function would be a cleaner solution, as proposed in this comment

I agree on that, but since the 2nd argument is already a component name, it will force you to use a name even if you don't need one.

I'll open the PR on the new codebase. Thank you

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding withAITracking will break styling for components (e.g. full width/height)
2 participants